Skip to content

Squash merge messages contain information about all commits by default. #35247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Squash merge messages contain information about all commits by default. #35247

wants to merge 24 commits into from
+174 −36

Conversation

hiifong
Copy link
Member

@hiifong hiifong commented Aug 11, 2025 •
edited
Loading

This behavior is basically the same as GitHub, except for the dividing line I added.

screenshot:

image image image image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 11, 2025
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations labels Aug 11, 2025
@hiifong hiifong closed this Aug 11, 2025
@hiifong hiifong reopened this Aug 11, 2025
lunny
lunny reviewed Aug 11, 2025
View reviewed changes
@lunny lunny added this to the 1.25.0 milestone Aug 11, 2025
@wxiaoguang
Copy link
Contributor

Please split preparePullViewReviewAndMerge into small functions, but not introduce a new preparePullViewSquashMergeMsg to call everywhere.

And it needs tests to cover the changed logic (although old code doesn't have enough tests, new code should have when the logic is complex)

@wxiaoguang
Copy link
Contributor

This part seems not right, Co-xxx should be at the end.

image

@hiifong
Copy link
Member Author

hiifong commented Aug 11, 2025

This part seems not right, Co-xxx should be at the end.

image

Done.

image

@hiifong
Copy link
Member Author

hiifong commented Aug 11, 2025

Let me spend some time researching how to add tests.

@silverwind
Copy link
Member

image

Style nit: there should be no empty line between such metadata lines.

@hiifong hiifong marked this pull request as draft August 15, 2025 03:26
@hiifong
Merge branch 'main' into squash-merge-msg
9ce1cae 
* main:
  [skip ci] Updated translations via Crowdin
  Fix LFS range size header response (go-gitea#35277)
  Skip "parentsigned" check when the repo is empty (go-gitea#35292)
  [skip ci] Updated translations via Crowdin
  Fix GitHub release assets URL validation (go-gitea#35287)
  nix flake use go1.25 (go-gitea#35288)
  go1.25.0 (go-gitea#35262)
wxiaoguang
wxiaoguang reviewed Aug 18, 2025
View reviewed changes
@appleboy
Copy link
Member

@hiifong Thanks for your contribution. Any progress on this?

hiifong added 2 commits August 31, 2025 20:11
@hiifong
Add test
b0456cf
@hiifong
Merge branch 'main' into squash-merge-msg
4a0796a 
* main: (34 commits)
  when sorting issues by nearest due date, issues without due date should be sorted ascending (go-gitea#35267)
  [skip ci] Updated translations via Crowdin
  Upgrade xz to v0.5.15 (go-gitea#35377)
  Refactor db package (go-gitea#35380)
  Remove the duplicated function GetTags (go-gitea#35375)
  [skip ci] Updated translations via Crowdin
  Allow foreachref parser to grow its buffer (go-gitea#35365)
  Remove global context from db package (go-gitea#35371)
  Use gitrepo.SetDefaultBranch when set default branch of wiki repository (go-gitea#33911)
  Deleting branch could delete broken branch which has database record but git branch is missing (go-gitea#35360)
  Remove incorrect "db.DefaultContext" usages (go-gitea#35366)
  Refactor to use reflect.TypeFor (go-gitea#35370)
  [skip ci] Updated translations via Crowdin
  Remove wrong "git.DefaultContext" (go-gitea#35364)
  Fix context usages (go-gitea#35348)
  Instance signing rule `pubkey` should allow all public keys, not just GPG (go-gitea#35357)
  Allow deleting comment with content via API like web did (go-gitea#35346)
  Prevent duplicate actions email (go-gitea#35215)
  rename nightly docker tag for main branch (go-gitea#35344)
  Fix atom/rss mixed error (go-gitea#35345)
  ...

# Conflicts:
#	tests/integration/pull_merge_test.go
@hiifong hiifong marked this pull request as ready for review August 31, 2025 12:19
@hiifong
Copy link
Member Author

hiifong commented Aug 31, 2025

image

Style nit: there should be no empty line between such metadata lines.

Done,
image

@hiifong
Copy link
Member Author

hiifong commented Aug 31, 2025 •
edited
Loading

@hiifong Thanks for your contribution. Any progress on this?

already for review

@hiifong
Merge branch 'main' into squash-merge-msg
1f14255 
* main:
  [skip ci] Updated translations via Crowdin
  Use github.com/mholt/archives replace github.com/mholt/archiver (go-gitea#35390)
  Move some functions from package git to gitrepo (go-gitea#33910)
  Move git config/remote to gitrepo package and add global lock to resolve possible conflict when updating repository git config file (go-gitea#35151)
  Move HasWiki to repository service package (go-gitea#33912)
  Rename UpdateBranch API to RenameBranch API (go-gitea#35374)
@hiifong
fix test
3703c2c
@hiifong
fix test
7d53f05
@hiifong hiifong requested review from wxiaoguang and lunny September 2, 2025 03:14
wxiaoguang
wxiaoguang reviewed Sep 2, 2025
View reviewed changes
lunny
lunny reviewed Sep 2, 2025
View reviewed changes
routers/web/repo/issue_view.go
ctx.Data["DefaultSquashMergeMessage"] = defaultSquashMergeMessage
ctx.Data["DefaultSquashMergeBody"] = defaultSquashMergeBody
ctx.Data["DefaultSquashMergeBody"] = fmt.Sprintf("--------------------\n%s%s", commitsBuilder.String(), defaultSquashMergeBody)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --------------------\n should be ignored if the pull request body is empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done b09ada6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it still needs more tests, to test various edge cases.

The test can clearly test this function's logic.

The existing tests in this PR (parse HTML pages) seem quite fragile and difficult to read. I guess you will also have difficulty to understand your code after 3 months.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll continue to refine this pr over the weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny left review comments

@wxiaoguang wxiaoguang wxiaoguang left review comments

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.
6 participants
@hiifong @wxiaoguang @silverwind @appleboy @lunny @GiteaBot